[2/n] [ls-apis] return walked dependencies instead of using a callback#10458
Conversation
Created using spr 1.3.6-beta.1
| /// that package. | ||
| /// | ||
| /// A package reachable by more than one path appears once per path. | ||
| pub found: Vec<(&'a Package, DepPath)>, |
There was a problem hiding this comment.
Should I make this something like a BTreeMap<&'a Package, Vec<DepPath>>? I didn't feel like it made much of a difference either way.
There was a problem hiding this comment.
A package reachable by more than one path appears once per path.
Nothing would happen if we had a bug where these were duplicated by mistake? If not, then I don't see a specific need to use BTreeMap<&'a Package, Vec<DepPath>>.
I'm guessing we're optimising for performance here? If that's the case, maybe a little comment explaining that would be nice.
There was a problem hiding this comment.
Ah I actually did consider using BTreeMap<&'a Package, Vec<DepPath>> here. But I decided not to mostly to be as close to the original code as possible (which was semantically equal to returning a Vec). I could go either way on this, and certainly the BTreeMap expresses the intent a bit clearer.
Perf isn't an issue here.
There was a problem hiding this comment.
Actually I'll do one better and use an IdOrdMap to be even clearer
There was a problem hiding this comment.
Ah I forgot that we need to index by the package ID. Makes the case for IdOrdMap even stronger.
There was a problem hiding this comment.
(fwiw the reason against doing this would be to preserve DFS order -- that is not actually a concern here, since none of our use cases depend on the depth-first nature of the search. So I think this is an improvement. But if we did need to preserve the order, a Vec would be the right data structure.)
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Thanks for separating this out from the other PR. I have a minor question, but otherwise looks good!
| /// that package. | ||
| /// | ||
| /// A package reachable by more than one path appears once per path. | ||
| pub found: Vec<(&'a Package, DepPath)>, |
There was a problem hiding this comment.
A package reachable by more than one path appears once per path.
Nothing would happen if we had a bug where these were duplicated by mistake? If not, then I don't see a specific need to use BTreeMap<&'a Package, Vec<DepPath>>.
I'm guessing we're optimising for performance here? If that's the case, maybe a little comment explaining that would be nice.
Created using spr 1.3.6-beta.1
sunshowers
deleted the
sunshowers/spr/2n-ls-apis-return-walked-dependencies-instead-of-using-a-callback
branch
2 participants
We're going to return more data from this function (the set of omitted nodes that were seen) in an upcoming PR.
I tried out a few different approaches:
Based on the prototyping I feel like this ends up being the cleanest approach.
There are no functional changes in this PR.
Best reviewed in conjunction with its dependent PR: